Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More accurately convert colortemp to xy and hs #146

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

dn0sar
Copy link

@dn0sar dn0sar commented Apr 20, 2021

The current color conversion from color temperature to rgb distorts colors. For instance during the day whiter colors get shifted slightly towards magenta, while in the evening warmer color temperatures shift more towards red. This is a problem when using led bulbs that don't directly support color temperature but can for instance be set in xy or hs. This PR uses a more accurate conversion for color temperature by using the functions provided by the colour-science package.

color_diffs

This photo shows the effect of this PR. The light bulbs in the three rooms in the picture (left, middle, and right) have all been set with the hass-circardian_lighting component.

The room on the right has a light that directly accepts color temperatures as a setting. The color of the middle room comes from setting a RGB light with the current version of this repo (without this PR). On the room on the left we have again a RGB light, but set in xy with this PR applied. As it can be seen the color generated with this PR (room on the left) more accurately resembles the light generated by the native color temperature light bulb (room on the right).

@dn0sar dn0sar changed the base branch from main to dev May 29, 2021 10:50
@@ -299,7 +300,7 @@ def _calc_rgb(self):
return color_temperature_to_rgb(self._color_temperature())

def _calc_xy(self):
return color_RGB_to_xy(*self._calc_rgb())
return list(colour.temperature.CCT_to_xy_CIE_D(self._color_temperature()))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something that's bothered me too but adding additional package requirements has always caused problems in the past for users that have non "standard" installations. If the colour-science package fails to install then the entire CL integration will break. I think a better solution would be to wrap this in a try and if an exception occurs to use the current built-in method. I can make that change when I get a chance or feel free to handle it if you're so inclined.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the feedback. Yep, agree with your point, also because currently the packaged version of colour-science has a problem when running on raspberry-pi (this was fixed ~8 months ago on their repo, but it is not yet on the release version). I'll add this check by the end of the week :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the only method used is color.temperature.CCT_to_xy_CIE_D() another option would be to just extract the code for this method (https://github.com/colour-science/colour/blob/a412b670448534987fc25d3a1a09cedc2a3ea5f2/colour/temperature/cie_d.py#L127-L183) from the package and omit the rest of it.

This would completely ditch the dependency while still providing the same functionality.

@@ -352,7 +353,8 @@ async def _adjust_lights(self, lights, transition):
if not is_on(self.hass, light):
continue

service_data = {ATTR_ENTITY_ID: light, ATTR_TRANSITION: transition}
service_data = {ATTR_ENTITY_ID: light}
service_data[ATTR_TRANSITION] = transition
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an advantage to breaking this into two lines? Have you run into issues where service_data was not set because of an issue with light or transition?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah well spotted! I actually didn't intend to commit this, but I forgot to remove it after I tested it on my system. The problem is that IKEA light bulbs don't handle transition and color changes commands issued at the same time very well. Particularly, they don't change the color when this happens, so I just commented out the transition part on my system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants